Skip to content

Conversation

@dl3sdo
Copy link
Member

@dl3sdo dl3sdo commented Apr 27, 2025

The 'Find next' search failed if the object query returned an object with either a hidden or protected symbol since these kind of objects can't be added to an object selection.
Exclude all hidden and protected objects from the 'Find next' and 'Find all' search.

The 'Find next' search failed if the object query returned an object
with either a hidden or protected symbol since these kind of objects
can't be added to an object selection.
Exclude all hidden and protected objects from the 'Find next' and
'Find all' search.
@dg0yt dg0yt self-requested a review April 27, 2025 13:24
@dl3sdo
Copy link
Member Author

dl3sdo commented Apr 27, 2025

For the 'Find all' mechanism the hidden and protected objects were just ignored when adding to the object selection, whereas for the 'Find next' mechanism the outcome depended on the internal order of the objects:

FindObjects_Issue2371.mp4

@dg0yt
Copy link
Member

dg0yt commented Apr 27, 2025

Yeah, I got the problem. I'm a little bit slow with reviewing the changes 🙈

@dg0yt dg0yt added this to the v0.9.6 milestone May 4, 2025
@dg0yt dg0yt self-assigned this May 4, 2025
@dg0yt
Copy link
Member

dg0yt commented May 12, 2025

Made findNext and findAll more similar, both using applyOnMatchingObjects(..., query) now. Avoiding the second walk for findNext.

It is a little bit irritating that objects with protected symbols are skipped. But this can't be changed easily now. The PR is an improvements nevertheless.

@dl3sdo
Copy link
Member Author

dl3sdo commented May 12, 2025

@dg0yt: adding the std::cref around query again fixes the compiler issue for me (it's compiling however). CI stops for some targets.

}
};
map->getCurrentPart()->applyOnMatchingObjects(search, std::cref(query));
map->getCurrentPart()->applyOnAllObjects(search);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverting part of my change here: The current object might no longer match the query, but we still want to see it when determining the next match.

@dl3sdo
Copy link
Member Author

dl3sdo commented May 13, 2025

@dg0yt: I'm adding a test case.

@dg0yt
Copy link
Member

dg0yt commented May 13, 2025

I will probably revert most of b0dcb5a. const and slot were omitted intentionally.

@dl3sdo
Copy link
Member Author

dl3sdo commented May 13, 2025

I decided to not create another separate file but to add the test case to ObjectQueryTest.
It took me some time to find out that I could not make ObjectQueryTest a friend of MapFindFeature since ObjectQueryTest is in a different namespace. And changing the namespace will cause trouble with Q_INIT_RESOURCE(resources); doStaticInitializations();
I am surprised that the search operates in reverse order (this was already the situation before this PR).

dg0yt added 2 commits May 14, 2025 14:33
No slot markup, member functions are good enough.
No const on functions which hand out non-const member.
Restore original order in header which matched order in source.

Move the new functions for testing purpose from public to protected.
The test can access them via light-weight derived type.
@dg0yt dg0yt force-pushed the fix-find-feature branch from d5c18d7 to b223b89 Compare May 14, 2025 12:37
@dl3sdo
Copy link
Member Author

dl3sdo commented May 14, 2025

Thank you Kai for refactoring my test code! BTW: tools_t.cpp was the file I also had in mind to put the test in and from which I tried to take over some code.

@dg0yt
Copy link
Member

dg0yt commented May 14, 2025

The last commit is the result of being forced into refactoring...

We can make the testable functions public because they only need the controller and query, not the object state. (They might be member functions of controller, but you know ... I don't want to add more capabilities to that class.)

@dl3sdo
Copy link
Member Author

dl3sdo commented May 14, 2025

@dg0yt: I checked the code once more and everything looks good to me. I really like the clear structure and especially the search functor which does now everything in just one function! Thanks again.

@dg0yt
Copy link
Member

dg0yt commented May 14, 2025

A few more twists.
It is really good now ;-)

@dl3sdo
Copy link
Member Author

dl3sdo commented May 17, 2025

I tested the final version and reviewed the code once again, everything now really looks good and works as intended. Thank you Kai for spending the effort.
Rebasing #2374 upon that PR will be really fun ;-)
Not related to this PR: I wonder if anyone really uses the 'Find next' entry in the 'Edit' menu instead of doing the search in the 'Find...' dialog.

@dg0yt
Copy link
Member

dg0yt commented May 17, 2025

Not related to this PR: I wonder if anyone really uses the 'Find next' entry in the 'Edit' menu instead of doing the search in the 'Find...' dialog.

This entry is for searching with the dialog hiding parts of the map. There used to be also shortcuts IIRC.

@dg0yt dg0yt merged commit 7dc7cfe into OpenOrienteering:master May 17, 2025
12 checks passed
@dg0yt
Copy link
Member

dg0yt commented May 17, 2025

Thanks Matthias.

@dl3sdo dl3sdo deleted the fix-find-feature branch May 17, 2025 18:24
dl3sdo added a commit to dl3sdo/mapper that referenced this pull request May 17, 2025
dl3sdo added a commit to dl3sdo/mapper that referenced this pull request Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants